Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CNV-52722: Pass through extra VDDK configuration options to importer pod. #3572

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/datavolumes.md
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,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 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)

## 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.

Expand Down
22 changes: 22 additions & 0 deletions manifests/example/vddk-args-annotation.yaml
Original file line number Diff line number Diff line change
@@ -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"
9 changes: 9 additions & 0 deletions manifests/example/vddk-args-configmap.yaml
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions pkg/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ 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"
// 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"
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,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"
Expand Down
26 changes: 26 additions & 0 deletions pkg/controller/import-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ type importerPodArgs struct {
imagePullSecrets []corev1.LocalObjectReference
workloadNodePlacement *sdkapi.NodePlacement
vddkImageName *string
vddkExtraArgs *string
priorityClassName string
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -523,6 +530,7 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim)
pvc: pvc,
scratchPvcName: scratchPvcName,
vddkImageName: vddkImageName,
vddkExtraArgs: vddkExtraArgs,
priorityClassName: cc.GetPriorityClass(pvc),
}

Expand Down Expand Up @@ -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: common.VddkArgsVolName,
MountPath: common.VddkArgsDir,
})
}
if args.podEnvVar.certConfigMap != "" {
containers[0].VolumeMounts = append(containers[0].VolumeMounts, corev1.VolumeMount{
Name: CertVolName,
Expand Down Expand Up @@ -1070,6 +1084,18 @@ func makeImporterVolumeSpec(args *importerPodArgs) []corev1.Volume {
},
})
}
if args.vddkExtraArgs != nil {
volumes = append(volumes, corev1.Volume{
Name: common.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))
}
Expand Down
30 changes: 30 additions & 0 deletions pkg/controller/import-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
23 changes: 23 additions & 0 deletions pkg/image/nbdkit.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,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,
Expand Down Expand Up @@ -228,6 +235,22 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just look for the first file just note we need to be sure to document this so user does not chain the configs to separate configmaps.

// pass that through nbdkit via the "config=" option.
func getVddkConfig() (string, error) {
path := filepath.Join(common.VddkArgsDir, common.VddkArgsKeyName)
_, err := os.Stat(path)
if err != nil {
if os.IsNotExist(err) { // No configuration file found, so no extra arguments to give to VDDK
return "", nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: Please add a comment that the user did not specify the vddk additional config

}
return "", err
}

return path, nil
}

func (n *Nbdkit) getSourceArg(s string) string {
var source string
switch n.plugin {
Expand Down
43 changes: 43 additions & 0 deletions tests/datavolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1184,6 +1184,25 @@ var _ = Describe("[vendor:[email protected]][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, "")
Expand Down Expand Up @@ -1254,6 +1273,30 @@ var _ = Describe("[vendor:[email protected]][level:component]DataVolume tests",
Message: "Import Complete",
Reason: "Completed",
}}),
Entry("[test_id:5083]succeed importing VDDK data volume with extra arguments ConfigMap set", dataVolumeTestArguments{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to assert something in the final step of this test? like checking the config was applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this test I wanted to make sure that the contents of the ConfigMap are present in the file, so the assertion happens in the vddk-test-plugin (the fgets/strcmp). Is there a better way to check the result from the importer? Like can this test read the pod logs or termination message?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh okay I see

if (strcmp(extras, "VixDiskLib.nfcAio.Session.BufSizeIn64KB=16") != 0) { // Must match datavolume_test
Yeah I think we can come up with a cleaner way,
You can either read pod logs or make this a part of the termination message struct
// GetTerminationMessage returns data to be serialized and used as the termination message of the importer.
func (vs *VDDKDataSource) GetTerminationMessage() *common.TerminationMessage {
return &common.TerminationMessage{
VddkInfo: &common.VddkInfo{
Version: vddkVersion,
Host: vddkHost,
},
}
}

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
Expand Down
21 changes: 21 additions & 0 deletions tools/vddk-test/vddk-test-plugin.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#define NBDKIT_API_VERSION 2
#include <nbdkit-plugin.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there plans to backport this? just making sure I have a maintenance path crafted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I asked around and I guess the customer looking for this is currently on 4.15. So we will definitely be asking for another barrage of backports.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up. Is there any way to avoid the test plugin change? I guess not. I am just asking since it could spare making new test images for all releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize this was an issue, the test is definitely not set up to help avoid generating images. I'm not sure how else to do it though, short of hiding the plugin in the importer image or something.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine

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;
}

Expand Down