From aadd62327d0237f1db56192fa5f11843bdbb8f38 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 16 Dec 2024 11:13:18 -0500 Subject: [PATCH 01/14] Add annotation for extra VDDK library arguments. The VDDK library itself accepts infrequently-used arguments in a configuration file, and some of these arguments have been tested to show a significant transfer speedup in some environments. This adds an annotation that references a ConfigMap holding the contents of this VDDK configuration file, and mounts it to the importer pod. The first file in the mounted directory is passed to the VDDK. Signed-off-by: Matthew Arnold --- pkg/common/common.go | 2 ++ pkg/controller/common/util.go | 2 ++ pkg/controller/import-controller.go | 26 +++++++++++++++++++++++ pkg/controller/util.go | 3 +++ pkg/image/nbdkit.go | 32 +++++++++++++++++++++++++++++ 5 files changed, 65 insertions(+) diff --git a/pkg/common/common.go b/pkg/common/common.go index 3e948fca63..6660f4afbf 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -254,6 +254,8 @@ const ( VddkConfigDataKey = "vddk-init-image" // AwaitingVDDK is a Pending condition reason that indicates the PVC is waiting for a VDDK image AwaitingVDDK = "AwaitingVDDK" + // VddkArgsDir is the path to the volume mount containing extra VDDK arguments + VddkArgsDir = "/vddk-args" // UploadContentTypeHeader is the header upload clients may use to set the content type explicitly UploadContentTypeHeader = "x-cdi-content-type" diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index 48c73628dd..816e359965 100644 --- a/pkg/controller/common/util.go +++ b/pkg/controller/common/util.go @@ -149,6 +149,8 @@ const ( AnnVddkHostConnection = AnnAPIGroup + "/storage.pod.vddk.host" // AnnVddkInitImageURL saves a per-DV VDDK image URL on the PVC AnnVddkInitImageURL = AnnAPIGroup + "/storage.pod.vddk.initimageurl" + // AnnVddkExtraArgs references a ConfigMap that holds arguments to pass directly to the VDDK library + AnnVddkExtraArgs = AnnAPIGroup + "/storage.pod.vddk.extraargs" // AnnRequiresScratch provides a const for our PVC requiring scratch annotation AnnRequiresScratch = AnnAPIGroup + "/storage.import.requiresScratch" diff --git a/pkg/controller/import-controller.go b/pkg/controller/import-controller.go index 49f1ff898f..364780eea1 100644 --- a/pkg/controller/import-controller.go +++ b/pkg/controller/import-controller.go @@ -116,6 +116,7 @@ type importerPodArgs struct { imagePullSecrets []corev1.LocalObjectReference workloadNodePlacement *sdkapi.NodePlacement vddkImageName *string + vddkExtraArgs *string priorityClassName string } @@ -480,6 +481,7 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim) r.log.V(1).Info("Creating importer POD for PVC", "pvc.Name", pvc.Name) var scratchPvcName *string var vddkImageName *string + var vddkExtraArgs *string var err error requiresScratch := r.requiresScratchSpace(pvc) @@ -508,6 +510,11 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim) } return errors.New(message) } + + if extraArgs, ok := anno[cc.AnnVddkExtraArgs]; ok && extraArgs != "" { + r.log.V(1).Info("Mounting extra VDDK args ConfigMap to importer pod", "ConfigMap", extraArgs) + vddkExtraArgs = &extraArgs + } } podEnvVar, err := r.createImportEnvVar(pvc) @@ -523,6 +530,7 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim) pvc: pvc, scratchPvcName: scratchPvcName, vddkImageName: vddkImageName, + vddkExtraArgs: vddkExtraArgs, priorityClassName: cc.GetPriorityClass(pvc), } @@ -999,6 +1007,12 @@ func makeImporterContainerSpec(args *importerPodArgs) []corev1.Container { MountPath: "/opt", }) } + if args.vddkExtraArgs != nil { + containers[0].VolumeMounts = append(containers[0].VolumeMounts, corev1.VolumeMount{ + Name: VddkArgsVolName, + MountPath: common.VddkArgsDir, + }) + } if args.podEnvVar.certConfigMap != "" { containers[0].VolumeMounts = append(containers[0].VolumeMounts, corev1.VolumeMount{ Name: CertVolName, @@ -1070,6 +1084,18 @@ func makeImporterVolumeSpec(args *importerPodArgs) []corev1.Volume { }, }) } + if args.vddkExtraArgs != nil { + volumes = append(volumes, corev1.Volume{ + Name: VddkArgsVolName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &v1.ConfigMapVolumeSource{ + LocalObjectReference: v1.LocalObjectReference{ + Name: *args.vddkExtraArgs, + }, + }, + }, + }) + } if args.podEnvVar.certConfigMap != "" { volumes = append(volumes, createConfigMapVolume(CertVolName, args.podEnvVar.certConfigMap)) } diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 81e050464d..d5301ccd25 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -52,6 +52,9 @@ const ( //nolint:gosec // This is not a real secret SecretVolName = "cdi-secret-vol" + // VddkArgsVolName is the name of the volume referencing the extra VDDK arguments ConfigMap + VddkArgsVolName = "vddk-extra-args" + // AnnOwnerRef is used when owner is in a different namespace AnnOwnerRef = cc.AnnAPIGroup + "/storage.ownerRef" diff --git a/pkg/image/nbdkit.go b/pkg/image/nbdkit.go index 21688b0e2c..78443e6eba 100644 --- a/pkg/image/nbdkit.go +++ b/pkg/image/nbdkit.go @@ -4,6 +4,7 @@ import ( "bufio" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -168,6 +169,13 @@ func NewNbdkitVddk(nbdkitPidFile, socket string, args NbdKitVddkPluginArgs) (Nbd pluginArgs = append(pluginArgs, "-D", "nbdkit.backend.datapath=0") pluginArgs = append(pluginArgs, "-D", "vddk.datapath=0") pluginArgs = append(pluginArgs, "-D", "vddk.stats=1") + config, err := getVddkConfig() + if err != nil { + return nil, err + } + if config != "" { + pluginArgs = append(pluginArgs, "config="+config) + } p := getVddkPluginPath() n := &Nbdkit{ NbdPidFile: nbdkitPidFile, @@ -228,6 +236,30 @@ func getVddkPluginPath() NbdkitPlugin { return NbdkitVddkPlugin } +// Extra VDDK configuration options are stored in a ConfigMap mounted to the +// importer pod. Just look for the first file in the mounted directory, and +// pass that through nbdkit via the "config=" option. +func getVddkConfig() (string, error) { + withHidden, err := os.ReadDir(common.VddkArgsDir) + if err != nil { + if os.IsNotExist(err) { + return "", nil + } + return "", err + } + files := []fs.DirEntry{} + for _, file := range withHidden { // Ignore hidden files + if !strings.HasPrefix(file.Name(), ".") { + files = append(files, file) + } + } + if len(files) < 1 { + return "", fmt.Errorf("no VDDK configuration files found under %s", common.VddkArgsDir) + } + path := filepath.Join(common.VddkArgsDir, files[0].Name()) + return path, nil +} + func (n *Nbdkit) getSourceArg(s string) string { var source string switch n.plugin { From 2ca6fa8b19089399ba910c2fdaa475874384a235 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 16 Dec 2024 11:53:32 -0500 Subject: [PATCH 02/14] Add functional test for VDDK args annotation. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 46 ++++++++++++++++++++++++++++++ tools/vddk-test/vddk-test-plugin.c | 21 ++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index dc1b531c7a..da222a6062 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1186,6 +1186,28 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", return dv } + createVddkDataVolumeWithExtraArgs := func(dataVolumeName, size, url string) *cdiv1.DataVolume { + dv := createVddkDataVolumeWithInitImageURL(dataVolumeName, size, url) + extraArguments := v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: f.Namespace.Name, + Name: "vddk-extra-args-map", + }, + Data: map[string]string{ // Must match vddk-test-plugin + "vddk-config-file": "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", + }, + } + _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), &extraArguments, metav1.CreateOptions{}) + if !k8serrors.IsAlreadyExists(err) { + Expect(err).ToNot(HaveOccurred()) + } + if dv.Annotations == nil { + dv.Annotations = make(map[string]string) + } + dv.Annotations[controller.AnnVddkExtraArgs] = extraArguments.Name + return dv + } + // Similar to previous table, but with additional cleanup steps to save and restore VDDK image config map DescribeTable("should", Serial, func(args dataVolumeTestArguments) { _, err := utils.CopyConfigMap(f.K8sClient, f.CdiInstallNs, common.VddkConfigMap, f.CdiInstallNs, savedVddkConfigMap, "") @@ -1256,6 +1278,30 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Message: "Import Complete", Reason: "Completed", }}), + Entry("[test_id:5083]succeed importing VDDK data volume with extra arguments ConfigMap set", dataVolumeTestArguments{ + name: "dv-import-vddk", + size: "1Gi", + url: vcenterURL, + dvFunc: createVddkDataVolumeWithExtraArgs, + eventReason: dvc.ImportSucceeded, + phase: cdiv1.Succeeded, + checkPermissions: false, + readyCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeReady, + Status: v1.ConditionTrue, + }, + boundCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeBound, + Status: v1.ConditionTrue, + Message: "PVC dv-import-vddk Bound", + Reason: "Bound", + }, + runningCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeRunning, + Status: v1.ConditionFalse, + Message: "Import Complete", + Reason: "Completed", + }}), ) // similar to other tables but with check of quota diff --git a/tools/vddk-test/vddk-test-plugin.c b/tools/vddk-test/vddk-test-plugin.c index 2d23ab9e30..7967c2d50d 100644 --- a/tools/vddk-test/vddk-test-plugin.c +++ b/tools/vddk-test/vddk-test-plugin.c @@ -13,6 +13,7 @@ #define NBDKIT_API_VERSION 2 #include #include +#include #include #include #include @@ -32,6 +33,26 @@ int fakevddk_config(const char *key, const char *value) { if (strcmp(key, "snapshot") == 0) { expected_arg_count = 9; // Expect one for 'snapshot' and one for 'transports' } + if (strcmp(key, "config") == 0) { + expected_arg_count = 8; + nbdkit_debug("Extra config option set to: %s\n", value); + + FILE *f = fopen(value, "r"); + if (f == NULL) { + nbdkit_error("Failed to open VDDK extra configuration file %s!\n", value); + return -1; + } + char extras[50]; + if (fgets(extras, 50, f) == NULL) { // Expect only one line of test data + nbdkit_error("Failed to read VDDK extra configuration file %s! Error was: %s", value, strerror(errno)); + return -1; + } + if (strcmp(extras, "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16") != 0) { // Must match datavolume_test + nbdkit_error("Unexpected content in VDDK extra configuration file %s: %s\n", value, extras); + return -1; + } + fclose(f); + } return 0; } From 39c1f385e9c42d494722e71d18596570187dbb23 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 16 Dec 2024 13:09:17 -0500 Subject: [PATCH 03/14] Add unit test for extra VDDK arguments annotation. Signed-off-by: Matthew Arnold --- pkg/controller/import-controller_test.go | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/controller/import-controller_test.go b/pkg/controller/import-controller_test.go index 96fb5e14a7..b2118cab62 100644 --- a/pkg/controller/import-controller_test.go +++ b/pkg/controller/import-controller_test.go @@ -935,6 +935,36 @@ var _ = Describe("Create Importer Pod", func() { Entry("with long PVC name", strings.Repeat("test-pvc-", 20), "snap1"), Entry("with long PVC and checkpoint names", strings.Repeat("test-pvc-", 20), strings.Repeat("repeating-checkpoint-id-", 10)), ) + + It("should mount extra VDDK arguments ConfigMap when annotation is set", func() { + pvcName := "testPvc1" + podName := "testpod" + extraArgs := "testing-123" + annotations := map[string]string{ + cc.AnnEndpoint: testEndPoint, + cc.AnnImportPod: podName, + cc.AnnSource: cc.SourceVDDK, + cc.AnnVddkInitImageURL: "testing-vddk", + cc.AnnVddkExtraArgs: extraArgs, + } + pvc := cc.CreatePvcInStorageClass(pvcName, "default", &testStorageClass, annotations, nil, corev1.ClaimBound) + reconciler := createImportReconciler(pvc) + + _, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: pvcName, Namespace: "default"}}) + Expect(err).ToNot(HaveOccurred()) + + pod := &corev1.Pod{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: podName, Namespace: "default"}, pod) + Expect(err).ToNot(HaveOccurred()) + + found := false // Look for vddk-args mount + for _, volume := range pod.Spec.Volumes { + if volume.ConfigMap != nil && volume.ConfigMap.Name == extraArgs { + found = true + } + } + Expect(found).To(BeTrue()) + }) }) var _ = Describe("Import test env", func() { From 13d6c7e1879699089fcc162b5eb92bc9ec2ca10d Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Tue, 17 Dec 2024 10:59:32 -0500 Subject: [PATCH 04/14] Add documentation for extra VDDK arguments. Signed-off-by: Matthew Arnold --- doc/datavolumes.md | 7 +++++++ manifests/example/vddk-args-annotation.yaml | 22 +++++++++++++++++++++ manifests/example/vddk-args-configmap.yaml | 9 +++++++++ pkg/image/nbdkit.go | 2 +- 4 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 manifests/example/vddk-args-annotation.yaml create mode 100644 manifests/example/vddk-args-configmap.yaml diff --git a/doc/datavolumes.md b/doc/datavolumes.md index 11415ef768..e74528c13b 100644 --- a/doc/datavolumes.md +++ b/doc/datavolumes.md @@ -365,6 +365,13 @@ spec: [Get VDDK ConfigMap example](../manifests/example/vddk-configmap.yaml) [Ways to find thumbprint](https://libguestfs.org/nbdkit-vddk-plugin.1.html#THUMBPRINTS) +#### Extra VDDK Configuration Options + +The VDDK library itself looks in a configuration file (such as `/etc/vmware/config`) for extra options to fine tune data transfers. To pass these options through to the VDDK, store the configuration file contents in a ConfigMap and add a `cdi.kubevirt.io/storage.pod.vddk.extraargs` annotation to the DataVolume specification. The ConfigMap will be mounted to the importer pod as a volume, and the first file in the mounted directory will be passed to the VDDK. This means that the ConfigMap must be placed in the same namespace as the DataVolume, and the ConfigMap should only have one file entry. + +[Example annotation](../manifests/example/vddk-args-annotation.yaml) +[Example ConfigMap](../manifests/example/vddk-args-configmap.yaml) + ## Multi-stage Import In a multi-stage import, multiple pods are started in succession to copy different parts of the source to an existing base disk image. Currently only the [ImageIO](#multi-stage-imageio-import) and [VDDK](#multi-stage-vddk-import) data sources support multi-stage imports. diff --git a/manifests/example/vddk-args-annotation.yaml b/manifests/example/vddk-args-annotation.yaml new file mode 100644 index 0000000000..fcada06358 --- /dev/null +++ b/manifests/example/vddk-args-annotation.yaml @@ -0,0 +1,22 @@ +apiVersion: cdi.kubevirt.io/v1beta1 +kind: DataVolume +metadata: + name: "vddk-dv" + namespace: "cdi" + annotations: + cdi.kubevirt.io/storage.pod.vddk.extraargs: vddk-arguments +spec: + source: + vddk: + backingFile: "[iSCSI_Datastore] vm/vm_1.vmdk" # From 'Hard disk'/'Disk File' in vCenter/ESX VM settings + url: "https://vcenter.corp.com" + uuid: "52260566-b032-36cb-55b1-79bf29e30490" + thumbprint: "20:6C:8A:5D:44:40:B3:79:4B:28:EA:76:13:60:90:6E:49:D9:D9:A3" # SSL fingerprint of vCenter/ESX host + secretRef: "vddk-credentials" + initImageURL: "registry:5000/vddk-init:latest" + storage: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: "32Gi" \ No newline at end of file diff --git a/manifests/example/vddk-args-configmap.yaml b/manifests/example/vddk-args-configmap.yaml new file mode 100644 index 0000000000..278918969e --- /dev/null +++ b/manifests/example/vddk-args-configmap.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + namespace: cdi + name: vddk-arguments +data: + vddk-config-file: -| + VixDiskLib.nfcAio.Session.BufSizeIn64KB=16 + VixDiskLib.nfcAio.Session.BufCount=4 \ No newline at end of file diff --git a/pkg/image/nbdkit.go b/pkg/image/nbdkit.go index 78443e6eba..d863edb54e 100644 --- a/pkg/image/nbdkit.go +++ b/pkg/image/nbdkit.go @@ -242,7 +242,7 @@ func getVddkPluginPath() NbdkitPlugin { func getVddkConfig() (string, error) { withHidden, err := os.ReadDir(common.VddkArgsDir) if err != nil { - if os.IsNotExist(err) { + if os.IsNotExist(err) { // No extra arguments ConfigMap specified, so mount directory does not exist return "", nil } return "", err From 8f57c87b2c27c93596e822f316350201f08e644b Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Wed, 18 Dec 2024 11:06:51 -0500 Subject: [PATCH 05/14] Simplify new functional test annotation creation. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index da222a6062..85201ebd8b 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1201,10 +1201,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", if !k8serrors.IsAlreadyExists(err) { Expect(err).ToNot(HaveOccurred()) } - if dv.Annotations == nil { - dv.Annotations = make(map[string]string) - } - dv.Annotations[controller.AnnVddkExtraArgs] = extraArguments.Name + controller.AddAnnotation(dv, controller.AnnVddkExtraArgs, extraArguments.Name) return dv } From 2d6e0bc4059e4b8488c39415a37caad7bd6ce1c6 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Fri, 20 Dec 2024 12:09:59 -0500 Subject: [PATCH 06/14] Look for specific file instead of first file. Instead of listing the mounted VDDK arguments directory and filtering out hidden files, just hard-code the expected file name and ConfigMap key. Signed-off-by: Matthew Arnold --- doc/datavolumes.md | 2 +- pkg/common/common.go | 4 ++++ pkg/controller/import-controller.go | 4 ++-- pkg/controller/util.go | 3 --- pkg/image/nbdkit.go | 17 ++++------------- tests/datavolume_test.go | 2 +- 6 files changed, 12 insertions(+), 20 deletions(-) diff --git a/doc/datavolumes.md b/doc/datavolumes.md index e74528c13b..78176bd79c 100644 --- a/doc/datavolumes.md +++ b/doc/datavolumes.md @@ -367,7 +367,7 @@ spec: #### Extra VDDK Configuration Options -The VDDK library itself looks in a configuration file (such as `/etc/vmware/config`) for extra options to fine tune data transfers. To pass these options through to the VDDK, store the configuration file contents in a ConfigMap and add a `cdi.kubevirt.io/storage.pod.vddk.extraargs` annotation to the DataVolume specification. The ConfigMap will be mounted to the importer pod as a volume, and the first file in the mounted directory will be passed to the VDDK. This means that the ConfigMap must be placed in the same namespace as the DataVolume, and the ConfigMap should only have one file entry. +The VDDK library itself looks in a configuration file (such as `/etc/vmware/config`) for extra options to fine tune data transfers. To pass these options through to the VDDK, store the configuration file contents in a ConfigMap with the key `vddk-config-file` and add a `cdi.kubevirt.io/storage.pod.vddk.extraargs` annotation to the DataVolume specification. The ConfigMap will be mounted to the importer pod as a volume, and the mount directory will have a file named `vddk-config-file` with the contents of the file. This means that the ConfigMap must be placed in the same namespace as the DataVolume, and the ConfigMap should only have one file entry, `vddk-config-file`. [Example annotation](../manifests/example/vddk-args-annotation.yaml) [Example ConfigMap](../manifests/example/vddk-args-configmap.yaml) diff --git a/pkg/common/common.go b/pkg/common/common.go index 6660f4afbf..be77b264d0 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -256,6 +256,10 @@ const ( AwaitingVDDK = "AwaitingVDDK" // VddkArgsDir is the path to the volume mount containing extra VDDK arguments VddkArgsDir = "/vddk-args" + // VddkArgsVolName is the name of the volume referencing the extra VDDK arguments ConfigMap + VddkArgsVolName = "vddk-extra-args" + // VddkArgsKeyName is the name of the key that must be present in the VDDK arguments ConfigMap + VddkArgsKeyName = "vddk-config-file" // UploadContentTypeHeader is the header upload clients may use to set the content type explicitly UploadContentTypeHeader = "x-cdi-content-type" diff --git a/pkg/controller/import-controller.go b/pkg/controller/import-controller.go index 364780eea1..225117308d 100644 --- a/pkg/controller/import-controller.go +++ b/pkg/controller/import-controller.go @@ -1009,7 +1009,7 @@ func makeImporterContainerSpec(args *importerPodArgs) []corev1.Container { } if args.vddkExtraArgs != nil { containers[0].VolumeMounts = append(containers[0].VolumeMounts, corev1.VolumeMount{ - Name: VddkArgsVolName, + Name: common.VddkArgsVolName, MountPath: common.VddkArgsDir, }) } @@ -1086,7 +1086,7 @@ func makeImporterVolumeSpec(args *importerPodArgs) []corev1.Volume { } if args.vddkExtraArgs != nil { volumes = append(volumes, corev1.Volume{ - Name: VddkArgsVolName, + Name: common.VddkArgsVolName, VolumeSource: corev1.VolumeSource{ ConfigMap: &v1.ConfigMapVolumeSource{ LocalObjectReference: v1.LocalObjectReference{ diff --git a/pkg/controller/util.go b/pkg/controller/util.go index d5301ccd25..81e050464d 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -52,9 +52,6 @@ const ( //nolint:gosec // This is not a real secret SecretVolName = "cdi-secret-vol" - // VddkArgsVolName is the name of the volume referencing the extra VDDK arguments ConfigMap - VddkArgsVolName = "vddk-extra-args" - // AnnOwnerRef is used when owner is in a different namespace AnnOwnerRef = cc.AnnAPIGroup + "/storage.ownerRef" diff --git a/pkg/image/nbdkit.go b/pkg/image/nbdkit.go index d863edb54e..fd5d1eb055 100644 --- a/pkg/image/nbdkit.go +++ b/pkg/image/nbdkit.go @@ -4,7 +4,6 @@ import ( "bufio" "fmt" "io" - "io/fs" "os" "os/exec" "path/filepath" @@ -240,23 +239,15 @@ func getVddkPluginPath() NbdkitPlugin { // importer pod. Just look for the first file in the mounted directory, and // pass that through nbdkit via the "config=" option. func getVddkConfig() (string, error) { - withHidden, err := os.ReadDir(common.VddkArgsDir) + path := filepath.Join(common.VddkArgsDir, common.VddkArgsKeyName) + _, err := os.Stat(path) if err != nil { - if os.IsNotExist(err) { // No extra arguments ConfigMap specified, so mount directory does not exist + if os.IsNotExist(err) { // No configuration file found, so no extra arguments to give to VDDK return "", nil } return "", err } - files := []fs.DirEntry{} - for _, file := range withHidden { // Ignore hidden files - if !strings.HasPrefix(file.Name(), ".") { - files = append(files, file) - } - } - if len(files) < 1 { - return "", fmt.Errorf("no VDDK configuration files found under %s", common.VddkArgsDir) - } - path := filepath.Join(common.VddkArgsDir, files[0].Name()) + return path, nil } diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index 85201ebd8b..fd4153f7fc 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1194,7 +1194,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Name: "vddk-extra-args-map", }, Data: map[string]string{ // Must match vddk-test-plugin - "vddk-config-file": "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", + common.VddkArgsKeyName: "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", }, } _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), &extraArguments, metav1.CreateOptions{}) From 85ee05b8c8bfc424dcec9b5a521fa8422723f0f0 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Tue, 7 Jan 2025 16:59:02 -0500 Subject: [PATCH 07/14] Move extra VDDK arguments functional test. Put this in import_test and assert the values there, instead of in the VDDK test plugin. The VDDK plugin logs the given values, and then the test scans the log for what it expects to see. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 43 ---------------------- tests/framework/vddk.go | 32 +++++++++++++++++ tests/import_test.go | 57 ++++++++++++++++++++++++++++++ tools/vddk-test/vddk-test-plugin.c | 11 ++---- 4 files changed, 92 insertions(+), 51 deletions(-) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index fd4153f7fc..dc1b531c7a 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1186,25 +1186,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", return dv } - createVddkDataVolumeWithExtraArgs := func(dataVolumeName, size, url string) *cdiv1.DataVolume { - dv := createVddkDataVolumeWithInitImageURL(dataVolumeName, size, url) - extraArguments := v1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: f.Namespace.Name, - Name: "vddk-extra-args-map", - }, - Data: map[string]string{ // Must match vddk-test-plugin - common.VddkArgsKeyName: "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", - }, - } - _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), &extraArguments, metav1.CreateOptions{}) - if !k8serrors.IsAlreadyExists(err) { - Expect(err).ToNot(HaveOccurred()) - } - controller.AddAnnotation(dv, controller.AnnVddkExtraArgs, extraArguments.Name) - return dv - } - // Similar to previous table, but with additional cleanup steps to save and restore VDDK image config map DescribeTable("should", Serial, func(args dataVolumeTestArguments) { _, err := utils.CopyConfigMap(f.K8sClient, f.CdiInstallNs, common.VddkConfigMap, f.CdiInstallNs, savedVddkConfigMap, "") @@ -1275,30 +1256,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Message: "Import Complete", Reason: "Completed", }}), - Entry("[test_id:5083]succeed importing VDDK data volume with extra arguments ConfigMap set", dataVolumeTestArguments{ - name: "dv-import-vddk", - size: "1Gi", - url: vcenterURL, - dvFunc: createVddkDataVolumeWithExtraArgs, - eventReason: dvc.ImportSucceeded, - phase: cdiv1.Succeeded, - checkPermissions: false, - readyCondition: &cdiv1.DataVolumeCondition{ - Type: cdiv1.DataVolumeReady, - Status: v1.ConditionTrue, - }, - boundCondition: &cdiv1.DataVolumeCondition{ - Type: cdiv1.DataVolumeBound, - Status: v1.ConditionTrue, - Message: "PVC dv-import-vddk Bound", - Reason: "Bound", - }, - runningCondition: &cdiv1.DataVolumeCondition{ - Type: cdiv1.DataVolumeRunning, - Status: v1.ConditionFalse, - Message: "Import Complete", - Reason: "Completed", - }}), ) // similar to other tables but with check of quota diff --git a/tests/framework/vddk.go b/tests/framework/vddk.go index b752271a5e..2f9e873e12 100644 --- a/tests/framework/vddk.go +++ b/tests/framework/vddk.go @@ -12,6 +12,38 @@ import ( "kubevirt.io/containerized-data-importer/tests/utils" ) +// CreateVddkDataVolume returns a VDDK data volume +func (f *Framework) CreateVddkDataVolume(dataVolumeName, size, url string) *cdiv1.DataVolume { + // Find vcenter-simulator pod + pod, err := utils.FindPodByPrefix(f.K8sClient, f.CdiInstallNs, "vcenter-deployment", "app=vcenter") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(pod).ToNot(gomega.BeNil()) + + // Get test VM UUID + id, err := f.RunKubectlCommand("exec", "-n", pod.Namespace, pod.Name, "--", "cat", "/tmp/vmid") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + vmid, err := uuid.Parse(strings.TrimSpace(id)) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Get disk name + disk, err := f.RunKubectlCommand("exec", "-n", pod.Namespace, pod.Name, "--", "cat", "/tmp/vmdisk") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + disk = strings.TrimSpace(disk) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Create VDDK login secret + stringData := map[string]string{ + common.KeyAccess: "user", + common.KeySecret: "pass", + } + backingFile := disk + secretRef := "vddksecret" + thumbprint := "testprint" + s, _ := utils.CreateSecretFromDefinition(f.K8sClient, utils.NewSecretDefinition(nil, stringData, nil, f.Namespace.Name, secretRef)) + + return utils.NewDataVolumeWithVddkImport(dataVolumeName, size, backingFile, s.Name, thumbprint, url, vmid.String()) +} + // CreateVddkWarmImportDataVolume fetches snapshot information from vcsim and returns a multi-stage VDDK data volume func (f *Framework) CreateVddkWarmImportDataVolume(dataVolumeName, size, url string) *cdiv1.DataVolume { // Find vcenter-simulator pod diff --git a/tests/import_test.go b/tests/import_test.go index bb345488db..b813524f88 100644 --- a/tests/import_test.go +++ b/tests/import_test.go @@ -192,6 +192,63 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:compo Expect(importer.DeletionTimestamp).To(BeNil()) } }) + + It("[test_id:6689]succeed importing VDDK data volume with extra arguments ConfigMap set", Label("VDDK"), func() { + vddkConfigOptions := []string{ + "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", + "vixDiskLib.nfcAio.Session.BufCount=4", + } + + vddkConfigMap := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vddk-extras", + }, + Data: map[string]string{ + common.VddkArgsKeyName: strings.Join(vddkConfigOptions, "\n"), + }, + } + + _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), vddkConfigMap, metav1.CreateOptions{}) + if !k8serrors.IsAlreadyExists(err) { + Expect(err).ToNot(HaveOccurred()) + } + + vcenterURL := fmt.Sprintf(utils.VcenterURL, f.CdiInstallNs) + dataVolume := f.CreateVddkDataVolume("import-pod-vddk-config-test", "100Mi", vcenterURL) + + By(fmt.Sprintf("Create new DataVolume %s", dataVolume.Name)) + controller.AddAnnotation(dataVolume, controller.AnnPodRetainAfterCompletion, "true") + controller.AddAnnotation(dataVolume, controller.AnnVddkExtraArgs, "vddk-extras") + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) + Expect(err).ToNot(HaveOccurred()) + + By("Verify PVC was created") + pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name) + Expect(err).ToNot(HaveOccurred()) + f.ForceBindIfWaitForFirstConsumer(pvc) + + By("Wait for import to be completed") + err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name) + Expect(err).ToNot(HaveOccurred(), "DataVolume not in phase succeeded in time") + + By("Find importer pods after completion") + pvcName := dataVolume.Name + // When using populators, the PVC Prime name is used to build the importer pod + if usePopulator, _ := dvc.CheckPVCUsingPopulators(pvc); usePopulator { + pvcName = populators.PVCPrimeName(pvc) + } + By("Find importer pod " + pvcName) + importer, err := utils.FindPodByPrefixOnce(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector) + Expect(err).ToNot(HaveOccurred()) + Expect(importer.DeletionTimestamp).To(BeNil()) + + logs, err := f.RunKubectlCommand("logs", "-n", dataVolume.Namespace, importer.Name) + Expect(err).To(BeNil()) + for _, option := range vddkConfigOptions { + By(fmt.Sprintf("Check for configuration value %s in nbdkit logs", option)) + Expect(strings.Contains(logs, option)).To(BeTrue()) + } + }) }) var _ = Describe("DataVolume Garbage Collection", Serial, func() { diff --git a/tools/vddk-test/vddk-test-plugin.c b/tools/vddk-test/vddk-test-plugin.c index 7967c2d50d..c0ccf788f3 100644 --- a/tools/vddk-test/vddk-test-plugin.c +++ b/tools/vddk-test/vddk-test-plugin.c @@ -42,14 +42,9 @@ int fakevddk_config(const char *key, const char *value) { nbdkit_error("Failed to open VDDK extra configuration file %s!\n", value); return -1; } - char extras[50]; - if (fgets(extras, 50, f) == NULL) { // Expect only one line of test data - nbdkit_error("Failed to read VDDK extra configuration file %s! Error was: %s", value, strerror(errno)); - return -1; - } - if (strcmp(extras, "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16") != 0) { // Must match datavolume_test - nbdkit_error("Unexpected content in VDDK extra configuration file %s: %s\n", value, extras); - return -1; + char extras[512]; // Importer test will scan debug log for given values, just pass them back + while (fgets(extras, 512, f) != NULL) { + nbdkit_debug("Extra configuration data: %s\n", extras); } fclose(f); } From e7791f60839e8b00223e9a0727be35d5a1e140fd Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Tue, 7 Jan 2025 17:20:54 -0500 Subject: [PATCH 08/14] Clean up lint error. Signed-off-by: Matthew Arnold --- tests/import_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/import_test.go b/tests/import_test.go index b813524f88..5cc2c5e0ef 100644 --- a/tests/import_test.go +++ b/tests/import_test.go @@ -243,7 +243,7 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:compo Expect(importer.DeletionTimestamp).To(BeNil()) logs, err := f.RunKubectlCommand("logs", "-n", dataVolume.Namespace, importer.Name) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) for _, option := range vddkConfigOptions { By(fmt.Sprintf("Check for configuration value %s in nbdkit logs", option)) Expect(strings.Contains(logs, option)).To(BeTrue()) From ca971b6e4e15f0cb9b5c143dbd6cc9f8456f2ea4 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Wed, 8 Jan 2025 10:03:10 -0500 Subject: [PATCH 09/14] Move VDDK configuration test back, change test ID. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 59 ++++++++++++++++++++++++++++++++++++++++ tests/framework/vddk.go | 32 ---------------------- tests/import_test.go | 57 -------------------------------------- 3 files changed, 59 insertions(+), 89 deletions(-) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index dc1b531c7a..dd976dce7c 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -3476,6 +3476,65 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", }), ) }) + + Describe("extra configuration options for VDDK imports", func() { + It("[test_id:XXXX]succeed importing VDDK data volume with extra arguments ConfigMap set", Label("VDDK"), func() { + vddkConfigOptions := []string{ + "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", + "vixDiskLib.nfcAio.Session.BufCount=4", + } + + vddkConfigMap := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vddk-extras", + }, + Data: map[string]string{ + common.VddkArgsKeyName: strings.Join(vddkConfigOptions, "\n"), + }, + } + + _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), vddkConfigMap, metav1.CreateOptions{}) + if !k8serrors.IsAlreadyExists(err) { + Expect(err).ToNot(HaveOccurred()) + } + + vcenterURL := fmt.Sprintf(utils.VcenterURL, f.CdiInstallNs) + dataVolume := createVddkDataVolume("import-pod-vddk-config-test", "100Mi", vcenterURL) + + By(fmt.Sprintf("Create new DataVolume %s", dataVolume.Name)) + controller.AddAnnotation(dataVolume, controller.AnnPodRetainAfterCompletion, "true") + controller.AddAnnotation(dataVolume, controller.AnnVddkExtraArgs, "vddk-extras") + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) + Expect(err).ToNot(HaveOccurred()) + + By("Verify PVC was created") + pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name) + Expect(err).ToNot(HaveOccurred()) + f.ForceBindIfWaitForFirstConsumer(pvc) + + By("Wait for import to be completed") + err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name) + Expect(err).ToNot(HaveOccurred(), "DataVolume not in phase succeeded in time") + + By("Find importer pods after completion") + pvcName := dataVolume.Name + // When using populators, the PVC Prime name is used to build the importer pod + if usePopulator, _ := dvc.CheckPVCUsingPopulators(pvc); usePopulator { + pvcName = populators.PVCPrimeName(pvc) + } + By("Find importer pod " + pvcName) + importer, err := utils.FindPodByPrefixOnce(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector) + Expect(err).ToNot(HaveOccurred()) + Expect(importer.DeletionTimestamp).To(BeNil()) + + logs, err := f.RunKubectlCommand("logs", "-n", dataVolume.Namespace, importer.Name) + Expect(err).ToNot(HaveOccurred()) + for _, option := range vddkConfigOptions { + By(fmt.Sprintf("Check for configuration value %s in nbdkit logs", option)) + Expect(strings.Contains(logs, option)).To(BeTrue()) + } + }) + }) }) func SetFilesystemOverhead(f *framework.Framework, globalOverhead, scOverhead string) { diff --git a/tests/framework/vddk.go b/tests/framework/vddk.go index 2f9e873e12..b752271a5e 100644 --- a/tests/framework/vddk.go +++ b/tests/framework/vddk.go @@ -12,38 +12,6 @@ import ( "kubevirt.io/containerized-data-importer/tests/utils" ) -// CreateVddkDataVolume returns a VDDK data volume -func (f *Framework) CreateVddkDataVolume(dataVolumeName, size, url string) *cdiv1.DataVolume { - // Find vcenter-simulator pod - pod, err := utils.FindPodByPrefix(f.K8sClient, f.CdiInstallNs, "vcenter-deployment", "app=vcenter") - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(pod).ToNot(gomega.BeNil()) - - // Get test VM UUID - id, err := f.RunKubectlCommand("exec", "-n", pod.Namespace, pod.Name, "--", "cat", "/tmp/vmid") - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - vmid, err := uuid.Parse(strings.TrimSpace(id)) - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - - // Get disk name - disk, err := f.RunKubectlCommand("exec", "-n", pod.Namespace, pod.Name, "--", "cat", "/tmp/vmdisk") - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - disk = strings.TrimSpace(disk) - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - - // Create VDDK login secret - stringData := map[string]string{ - common.KeyAccess: "user", - common.KeySecret: "pass", - } - backingFile := disk - secretRef := "vddksecret" - thumbprint := "testprint" - s, _ := utils.CreateSecretFromDefinition(f.K8sClient, utils.NewSecretDefinition(nil, stringData, nil, f.Namespace.Name, secretRef)) - - return utils.NewDataVolumeWithVddkImport(dataVolumeName, size, backingFile, s.Name, thumbprint, url, vmid.String()) -} - // CreateVddkWarmImportDataVolume fetches snapshot information from vcsim and returns a multi-stage VDDK data volume func (f *Framework) CreateVddkWarmImportDataVolume(dataVolumeName, size, url string) *cdiv1.DataVolume { // Find vcenter-simulator pod diff --git a/tests/import_test.go b/tests/import_test.go index 5cc2c5e0ef..bb345488db 100644 --- a/tests/import_test.go +++ b/tests/import_test.go @@ -192,63 +192,6 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:compo Expect(importer.DeletionTimestamp).To(BeNil()) } }) - - It("[test_id:6689]succeed importing VDDK data volume with extra arguments ConfigMap set", Label("VDDK"), func() { - vddkConfigOptions := []string{ - "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", - "vixDiskLib.nfcAio.Session.BufCount=4", - } - - vddkConfigMap := &v1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vddk-extras", - }, - Data: map[string]string{ - common.VddkArgsKeyName: strings.Join(vddkConfigOptions, "\n"), - }, - } - - _, err := f.K8sClient.CoreV1().ConfigMaps(f.Namespace.Name).Create(context.TODO(), vddkConfigMap, metav1.CreateOptions{}) - if !k8serrors.IsAlreadyExists(err) { - Expect(err).ToNot(HaveOccurred()) - } - - vcenterURL := fmt.Sprintf(utils.VcenterURL, f.CdiInstallNs) - dataVolume := f.CreateVddkDataVolume("import-pod-vddk-config-test", "100Mi", vcenterURL) - - By(fmt.Sprintf("Create new DataVolume %s", dataVolume.Name)) - controller.AddAnnotation(dataVolume, controller.AnnPodRetainAfterCompletion, "true") - controller.AddAnnotation(dataVolume, controller.AnnVddkExtraArgs, "vddk-extras") - dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) - Expect(err).ToNot(HaveOccurred()) - - By("Verify PVC was created") - pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name) - Expect(err).ToNot(HaveOccurred()) - f.ForceBindIfWaitForFirstConsumer(pvc) - - By("Wait for import to be completed") - err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name) - Expect(err).ToNot(HaveOccurred(), "DataVolume not in phase succeeded in time") - - By("Find importer pods after completion") - pvcName := dataVolume.Name - // When using populators, the PVC Prime name is used to build the importer pod - if usePopulator, _ := dvc.CheckPVCUsingPopulators(pvc); usePopulator { - pvcName = populators.PVCPrimeName(pvc) - } - By("Find importer pod " + pvcName) - importer, err := utils.FindPodByPrefixOnce(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector) - Expect(err).ToNot(HaveOccurred()) - Expect(importer.DeletionTimestamp).To(BeNil()) - - logs, err := f.RunKubectlCommand("logs", "-n", dataVolume.Namespace, importer.Name) - Expect(err).ToNot(HaveOccurred()) - for _, option := range vddkConfigOptions { - By(fmt.Sprintf("Check for configuration value %s in nbdkit logs", option)) - Expect(strings.Contains(logs, option)).To(BeTrue()) - } - }) }) var _ = Describe("DataVolume Garbage Collection", Serial, func() { From b8e51fc27ac5a85d6f5f35813ba22466f7ba06df Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Thu, 9 Jan 2025 10:19:45 -0500 Subject: [PATCH 10/14] Avoid using kubectl for scanning nbdkit logs. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index dd976dce7c..3bad7ddd28 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -14,10 +14,12 @@ import ( "github.com/google/uuid" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + core "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -3527,12 +3529,16 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Expect(err).ToNot(HaveOccurred()) Expect(importer.DeletionTimestamp).To(BeNil()) - logs, err := f.RunKubectlCommand("logs", "-n", dataVolume.Namespace, importer.Name) - Expect(err).ToNot(HaveOccurred()) - for _, option := range vddkConfigOptions { - By(fmt.Sprintf("Check for configuration value %s in nbdkit logs", option)) - Expect(strings.Contains(logs, option)).To(BeTrue()) - } + Eventually(func() (string, error) { + out, err := f.K8sClient.CoreV1(). + Pods(importer.Namespace). + GetLogs(importer.Name, &core.PodLogOptions{SinceTime: &meta.Time{Time: CurrentSpecReport().StartTime}}). + DoRaw(context.Background()) + return string(out), err + }, time.Minute, pollingInterval).Should(And( + ContainSubstring(vddkConfigOptions[0]), + ContainSubstring(vddkConfigOptions[1]), + )) }) }) }) From 14530056b69bd1bea7e47a87ecad872a23330c7f Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Thu, 9 Jan 2025 13:32:28 -0500 Subject: [PATCH 11/14] Temporary: show whole nbdkit log after failure. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index 3bad7ddd28..464e3f191c 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" "github.com/google/uuid" @@ -3481,6 +3482,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Describe("extra configuration options for VDDK imports", func() { It("[test_id:XXXX]succeed importing VDDK data volume with extra arguments ConfigMap set", Label("VDDK"), func() { + format.MaxLength = 0 vddkConfigOptions := []string{ "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", "vixDiskLib.nfcAio.Session.BufCount=4", From 057d450f12a5458e662d41ca762084dc7ea167f2 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 13 Jan 2025 13:29:07 -0500 Subject: [PATCH 12/14] Revert "Temporary: show whole nbdkit log after failure." This reverts commit 488849f8fd321d55a2b5d42a99838b11567d0029. Signed-off-by: Matthew Arnold --- tests/datavolume_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index 464e3f191c..3bad7ddd28 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -10,7 +10,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/onsi/gomega/format" "github.com/google/uuid" @@ -3482,7 +3481,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Describe("extra configuration options for VDDK imports", func() { It("[test_id:XXXX]succeed importing VDDK data volume with extra arguments ConfigMap set", Label("VDDK"), func() { - format.MaxLength = 0 vddkConfigOptions := []string{ "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16", "vixDiskLib.nfcAio.Session.BufCount=4", From 822bd133fecfa0fd184c7b132cba1d66779bb915 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 13 Jan 2025 14:09:45 -0500 Subject: [PATCH 13/14] Copy extra VDDK args annotation for populators. Also add a related unit test. Signed-off-by: Matthew Arnold --- .../populators/import-populator_test.go | 56 +++++++++++++++++++ pkg/controller/populators/populator-base.go | 3 + 2 files changed, 59 insertions(+) diff --git a/pkg/controller/populators/import-populator_test.go b/pkg/controller/populators/import-populator_test.go index b9c952dbb4..495a7d2475 100644 --- a/pkg/controller/populators/import-populator_test.go +++ b/pkg/controller/populators/import-populator_test.go @@ -501,6 +501,62 @@ var _ = Describe("Import populator tests", func() { Expect(pvcPrime.GetAnnotations()[AnnCurrentCheckpoint]).To(Equal("current")) Expect(pvcPrime.GetAnnotations()[AnnFinalCheckpoint]).To(Equal("true")) }) + + It("Should create PVC prime with proper VDDK import annotations", func() { + targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, map[string]string{}, nil, corev1.ClaimPending) + targetPvc.Spec.DataSourceRef = dataSourceRef + targetPvc.Annotations[AnnVddkExtraArgs] = "vddk-extras" + + volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault) + volumeImportSource.Spec.Source = &cdiv1.ImportSourceType{ + VDDK: &cdiv1.DataVolumeSourceVDDK{ + BackingFile: "testBackingFile", + SecretRef: "testSecret", + Thumbprint: "testThumbprint", + URL: "testUrl", + UUID: "testUUID", + }, + } + + By("Reconcile") + reconciler = createImportPopulatorReconciler(targetPvc, volumeImportSource, sc) + result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}}) + Expect(err).To(Not(HaveOccurred())) + Expect(result).To(Not(BeNil())) + + By("Checking events recorded") + close(reconciler.recorder.(*record.FakeRecorder).Events) + found := false + for event := range reconciler.recorder.(*record.FakeRecorder).Events { + if strings.Contains(event, createdPVCPrimeSuccessfully) { + found = true + } + } + reconciler.recorder = nil + Expect(found).To(BeTrue()) + + By("Checking PVC' annotations") + pvcPrime, err := reconciler.getPVCPrime(targetPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(pvcPrime).ToNot(BeNil()) + // make sure we didnt inflate size + Expect(pvcPrime.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("1G"))) + Expect(pvcPrime.GetAnnotations()).ToNot(BeNil()) + Expect(pvcPrime.GetAnnotations()[AnnImmediateBinding]).To(Equal("")) + Expect(pvcPrime.GetAnnotations()[AnnUploadRequest]).To(Equal("")) + Expect(pvcPrime.GetAnnotations()[AnnPopulatorKind]).To(Equal(cdiv1.VolumeImportSourceRef)) + Expect(pvcPrime.GetAnnotations()[AnnPreallocationRequested]).To(Equal("true")) + Expect(pvcPrime.GetAnnotations()[AnnBackingFile]).To(Equal("testBackingFile")) + Expect(pvcPrime.GetAnnotations()[AnnSecret]).To(Equal("testSecret")) + Expect(pvcPrime.GetAnnotations()[AnnThumbprint]).To(Equal("testThumbprint")) + Expect(pvcPrime.GetAnnotations()[AnnEndpoint]).To(Equal("testUrl")) + Expect(pvcPrime.GetAnnotations()[AnnUUID]).To(Equal("testUUID")) + Expect(pvcPrime.GetAnnotations()[AnnSource]).To(Equal(SourceVDDK)) + Expect(pvcPrime.GetLabels()[LabelExcludeFromVeleroBackup]).To(Equal("true")) + + Expect(pvcPrime.GetAnnotations()[AnnVddkExtraArgs]).To(Equal("vddk-extras")) + }) + }) var _ = Describe("Import populator progress report", func() { diff --git a/pkg/controller/populators/populator-base.go b/pkg/controller/populators/populator-base.go index 6c6fd8f8aa..34722921df 100644 --- a/pkg/controller/populators/populator-base.go +++ b/pkg/controller/populators/populator-base.go @@ -181,6 +181,9 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc if _, ok := pvc.Annotations[cc.AnnPodRetainAfterCompletion]; ok { annotations[cc.AnnPodRetainAfterCompletion] = pvc.Annotations[cc.AnnPodRetainAfterCompletion] } + if vddkExtraArgs, ok := pvc.Annotations[cc.AnnVddkExtraArgs]; ok && vddkExtraArgs != "" { + annotations[cc.AnnVddkExtraArgs] = vddkExtraArgs + } // Assemble PVC' spec pvcPrime := &corev1.PersistentVolumeClaim{ From 6ee8adbf772f0f433b7a94c706f61e6069527b74 Mon Sep 17 00:00:00 2001 From: Matthew Arnold Date: Mon, 13 Jan 2025 14:10:27 -0500 Subject: [PATCH 14/14] Correct VDDK args config map mount comment. Signed-off-by: Matthew Arnold --- pkg/image/nbdkit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/image/nbdkit.go b/pkg/image/nbdkit.go index fd5d1eb055..36ac713792 100644 --- a/pkg/image/nbdkit.go +++ b/pkg/image/nbdkit.go @@ -236,7 +236,7 @@ func getVddkPluginPath() NbdkitPlugin { } // Extra VDDK configuration options are stored in a ConfigMap mounted to the -// importer pod. Just look for the first file in the mounted directory, and +// importer pod. Make sure a "vddk-config-file" exists in that directory and // pass that through nbdkit via the "config=" option. func getVddkConfig() (string, error) { path := filepath.Join(common.VddkArgsDir, common.VddkArgsKeyName)